Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ddl: Fix exchange partition impl #8379

Closed
wants to merge 18 commits into from
Closed

ddl: Fix exchange partition impl #8379

wants to merge 18 commits into from

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Nov 16, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: close #8378

Problem Summary:

  • TableIDMap::emplaceTableID/TableIDMap::emplacePartitionTableID should check whether the old value is the same as new value before logging a warning message. Or there could be too many meanless warning logging.
  • When executing exchange partition, "renaming old non-partition table belonging new database" fail should not affect "renaming the exchanged partition table belonging new database"
  • The nesting if ... else ... in applySetTiFlashReplica is too deep and hard to read
  • Executing exchange partition in table id mapping is not atomic, this may make other functions get something wrong

  • tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

    Lines 128 to 131 in 1addc37

    table_id_map.eraseTableIDOrLogError(diff.old_table_id);
    table_id_map.emplaceTableID(diff.table_id, diff.schema_id);
    table_id_map.erasePartitionTableIDOrLogError(diff.table_id);
    table_id_map.emplacePartitionTableID(diff.old_table_id, diff.affected_opts[0].table_id);

What is changed and how it works?

  • Add some test case to master branch from Table can not be created after ALTER TABLE ... EXCHANGE PARTITION #8372
  • Make exchange partition to be an atomic function TableIDMap::exchangeTablePartition
  • TableIDMap::emplaceTableID/TableIDMap::emplacePartitionTableID check whether the old value is the same as new value before logging a warning message
  • Use do ... while ... break to make "renaming old non-partition table belonging new database" fail not affect "renaming the exchanged partition table belonging new database"
  • Adding return to simplify applySetTiFlashReplica

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 16, 2023
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 16, 2023
JaySon-Huang
JaySon-Huang commented Nov 16, 2023
View reviewed changes
dbms/src/TiDB/Schema/SchemaBuilder.cpp
@@ -317,68 +459,63 @@ void SchemaBuilder<Getter, NameMapper>::applySetTiFlashReplica(DatabaseID databa
}

applyDropTable(db_info->id, table_id);
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in applySetTiFlashReplica is adding return instead of nesting if ... else .... Make code more clear and simple.

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

Lloyd-Pottiger
Lloyd-Pottiger approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@Lloyd-Pottiger Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others LGTM

dbms/src/TiDB/Schema/SchemaBuilder.cpp Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 16, 2023
hongyunyan
hongyunyan approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@hongyunyan hongyunyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot bot added the lgtm label Nov 17, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongyunyan, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Lloyd-Pottiger,hongyunyan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 17, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 17, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-11-16 10:19:39.434118306 +0000 UTC m=+4330777.021228437: ☑️ agreed by Lloyd-Pottiger.
  • 2023-11-17 09:27:41.503363262 +0000 UTC m=+4414059.090473391: ☑️ agreed by hongyunyan.

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

/hold


[2023-11-20T03:00:29.762Z] fullstack-test2/ddl/alter_exchange_partition.test: Running
[2023-11-20T03:00:37.839Z]   File: fullstack-test2/ddl/alter_exchange_partition.test
[2023-11-20T03:00:37.840Z]   Error line: 49
[2023-11-20T03:00:37.840Z]   Error: set session tidb_isolation_read_engines='tiflash'; select * from test.e order by id;
[2023-11-20T03:00:37.840Z]   Result:
[2023-11-20T03:00:37.840Z]     ERROR 1105 (HY000) at line 1: other error for mpp stream: Code: 0, e.displayText() = DB::TiFlashException: The schema does not match the query, details: the column in the query is not found in current columns, keyspace=4294967295 table_id=593 column_id=4, e.what() = DB::TiFlashException,
[2023-11-20T03:00:37.840Z]   Expected:
[2023-11-20T03:00:37.840Z]     +-----+-------+-------+------+
[2023-11-20T03:00:37.840Z]     | id  | fname | lname | c1   |
[2023-11-20T03:00:37.840Z]     +-----+-------+-------+------+
[2023-11-20T03:00:37.840Z]     |   2 | a     | b     | NULL |
[2023-11-20T03:00:37.840Z]     | 108 | a     | b     | NULL |
[2023-11-20T03:00:37.840Z]     +-----+-------+-------+------+

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2023
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 21, 2023

@JaySon-Huang: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@Lloyd-Pottiger
Copy link
Contributor

/build

@JaySon-Huang
Copy link
Contributor Author

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2023
@JaySon-Huang JaySon-Huang force-pushed the fix_exchange_impl branch 4 times, most recently from db0784c to 742c0b9 Compare November 22, 2023 09:54
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 23, 2023
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 23, 2023
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

replace by #8421 and #8422

@JaySon-Huang JaySon-Huang deleted the fix_exchange_impl branch November 26, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lloyd-Pottiger Lloyd-Pottiger Lloyd-Pottiger approved these changes

@hongyunyan hongyunyan hongyunyan approved these changes

Assignees
No one assigned
Labels
approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential concurrency issue when executing exchange partition
3 participants
@JaySon-Huang @Lloyd-Pottiger @hongyunyan